I commented on one part that doesn't look quite right. Aside from that the patch looks okay, but this is just a minimal functionality hook. Is someone going to investigate merging the look-at functionality into one of the existing hierarchy traversals? It looks to me like all that's needed is to add submodel_look_at in the appropriate spot.

EDIT: well, okay, m!m said "I'm also not sure if my usage of model_look_at is right in model_render_queue" in the Mantis ticket, and I too am not sure if model_look_at was added to the right function. This needs testing.

I've gotten back in touch with Vasudan Admiral. He will provide the model and a test mission.

Once I have the test assets in hand, I plan to test both this patch as well as an alternative patch that merges the look_at functionality in an existing traversal.

I spent quite some time today looking at this, and I now have a better handle on this part of the code, although it's unclear exactly why we have both polymodels and polymodel_instances. I thought it was similar to ship_info and ship but it's not.

At the very least, we should be able to merge the look_at traversal into the dumb_rotation traversal; the two are almost identical. But I want to see how far down the rabbit hole this model_instance thing goes.

I think the ploymodel_instance contains information about the rotations of one instance (e.g. a specific ship) of the model.

That's what I thought initially, but the code is rotating submodels on the polymodel, not the polymodel_instance.

Yep, it's a rabbit hole. And it will require a bit of work to climb out of.

I will update again tomorrow.

I remember that polymodel_instance is a relatively new addition. Previously the submodels of the polymodel were changed but now most of the code uses the instance method. The look at code probably was written before that system was introduced.

You're right -- polymodel_instance does not appear in the original public release. Do you know why that was changed?

Copying the rotation information into the model was very performance intensive. Most operations (e.g. determining the global position of a point that is relative to a subobject) only needed the rotation information so that was separated from the rest and the simple computations could be done using only the instance.
However, I'm not entirely sure if that is the actual reason, it is just what I was told the last time I made a model related patch.

Made some progress, in consultation with Swifty. Some refactoring needed, but the end result should be more robust and should align with the design for the rest of the model code.

What's the status of the refactor? I took the idea of combining look_at and dumb rotations and implemented it here: master...asarium:mantis/3115_2

The dumb_rotation refactor is done; the look_at refactor is not far behind. You can see the status of dumb_rotation in my branch here:
https://github.com/Goober5000/fs2open.github.com/tree/dumb_rotation_refactor

Unfortunately, Swifty's time is limited, which is what is causing all the delays. One of the problems is that the original implementation of model_instances back in 2010 was only done for collision detection, not for rendering or other miscellaneous uses. Swifty added the rendering implementation in #486 and #496, so only a little bit more work remains. He said he'd give me a status update on Friday, but I haven't heard from him yet, so I'll ping him again.

The bottom line is that the refactoring is making excellent, albeit slow, progress. Once the prerequisites from Swifty are done, I'll merge in the dumb_rotation refactor. Then it should be a very small additional effort to complete the look_at refactor as well. The end result will be code that is in much better shape, without ugly and inefficient hacks to graft on specialized functionality.

Alright. Does it make sense to still include this in the 3.7.4 release? Without this we could release 3.7.4 soon and focus on stabilizing antipodes.

Urgh. I'd like to keep this in 3.7.4, simply because a) the look_at feature has been deferred for several releases, and it would be nice to finally actually include it; and b) this would tie up the submodel code with a nice bow instead of leaving things half-done.

I wish things were moving along faster myself, but we are making regular and consistent progress, and we have only a few steps remaining before this is complete. So I'd ask for a bit more time.

Swifty responded today. We ran into a problem which could have been avoided if the dumb_rotation feature had been coded properly in the first place. :-/ But we found a way around it and hopefully Swifty will be able to complete his prerequisite shortly.

Swifty submitted PR #501. Once that's reviewed, I should be able to add the dumb_rotation pretty quickly. The look_at change will take a little bit more time but should also be doable in the very short term.

Closing in favour of PR #530.